fix(config): match purchase suppressions across the ec2 and compute slugs - #1419

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/suppression-ec2-slug
• File: .coderabbit.yaml
• Commit: 41de606
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: eeb9928c-5c89-48ee-8df4-27c7ec3f9f31

Commits

Reviewing files that changed from the base of the PR and between 5aa3df5 and 41de606.

Files selected for processing: 6

• internal/config/suppressions_build.go
• internal/config/suppressions_build_test.go
• internal/config/types.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_suppressions_test.go
• internal/scheduler/suppression_variant_integration_test.go

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Merge Risk: Minimal · up to 41de6

No actionable merge-blocking risk is established for the EC2 suppression matching change.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the primary change: matching purchase suppressions across the AWS EC2 and compute service slugs.
Linked Issues check		The changes satisfy #1412. CanonicalSuppressionService maps AWS ec2 to compute. The scheduler normalizes stored suppressions and recommendations before matching. BuildSuppressions writes the c…
Out of Scope Changes check		The changed files support #1412. They implement service canonicalization, scheduler matching, suppression construction, and regression tests. The evidence shows no unrelated product behavior, SQL chan…
Docstring Coverage		Docstring coverage is 90.91% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 6 files.

Full details: Linked Issues check

Explanation

The changes satisfy #1412. CanonicalSuppressionService maps AWS ec2 to compute. The scheduler normalizes stored suppressions and recommendations before matching. BuildSuppressions writes the canonical AWS service. Unit and real-Postgres tests cover both spellings and preserve Azure and GCP behavior.

Full details: Out of Scope Changes check

Explanation

The changed files support #1412. They implement service canonicalization, scheduler matching, suppression construction, and regression tests. The evidence shows no unrelated product behavior, SQL change, or index change.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

Independent review 2 (claude-fleet, cold): APPROVE 41de606

Findings:
Blocking: none.
Non-blocking:

1. The same bug class exists for the other AWS alias pairs, and this PR does not fix it. The pinned AWS provider treats rds/relational-db, elasticache/cache, opensearch/search and redshift/data-warehouse as the same service (providers/aws provider.go:468-476, GetSupportedServices:434-453). internal/purchase/execution.go:1268-1281 (mapServiceSlug) accepts both spellings for AWS. internal/api/validation.go:599 only trims the service. So a purchase sent as aws/relational-db writes a suppression that does not hide the scheduler's aws/rds rec, which is the same double-buy as fix(config): purchase suppressions miss recs stored under the other EC2 service slug #1412. The scheduler collects legacy slugs only (scheduler.go:1106), so this needs an API client that sends canonical slugs. fix(config): purchase suppressions miss recs stored under the other EC2 service slug #1412 is scoped to EC2, so this is not a blocker for this PR. Suggest a follow-up issue to generalise CanonicalSuppressionService (types.go:766) to every AWS legacy/canonical pair.
2. internal/config/types.go:767 uses the literals "aws", "ec2" and "compute". The code next to it (suppressions_build.go:46) uses common.ProviderAWS, common.ServiceEC2 and common.ServiceCompute. This is a nit.
3. The provider comparison is case-sensitive. I checked whether that matters: the request side lowercases the provider (validation.go:593-598), collection uses the lowercase providerName, and retry and plan-step paths reuse already-validated or stored recs. Before this PR the key was exact-match too. So this is not a defect.

Consumer table (site / canonicalised? / evidence):

• scheduler aggregateSuppressions (suppression side) / yes / scheduler.go:1429
• scheduler applySuppressionIndex (rec side) / yes / scheduler.go:1475
• BuildSuppressions write bucket / yes / suppressions_build.go:35
• fix(purchase): suppress every variant when a legacy EC2 recommendation has no details #1411 wildcard / yes, reads the canonical bucket / suppressions_build.go:46. For aws this is equivalent to the base check ec2||compute, and azure and gcp stay false.
• Writers: attach_recommendations.go:35, plan_step_attach.go:348, handler_purchases.go:2602 and :3585 / all go through BuildSuppressions / grep
• SQL: ListActiveSuppressions has no service filter (store_postgres_suppressions.go:159-170). The deletes are by execution_id only (:147, store_postgres.go:909). No SQL compares service, so no SQL change is needed.
• Frontend / only the badge (recommendations.ts:3637). No service matching.
• Legacy "ec2" rows / matched on read / both sides canonicalise at match time. The integration test inserts raw "ec2" and "compute" rows.
• Not collapsed: azure and gcp compute, aws rds, savingsplans and savings-plans-compute. Each returns its service unchanged, and the provider is part of the helper and of the key.
• Account and region stay in the key. az_config is still absent from the key; that predates this PR (fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239) and is out of scope.
• Double canonicalisation is idempotent: compute maps to compute.

Probes (scratch copy of the head export; each file restored with cp and cmp-verified against the export):

• P1, rec side only (sup side raw): 9 scheduler tests FAIL, including EC2SlugSpellings/aws_ec2_sup_hides_compute_rec.
• P2, suppression side only: 10 scheduler tests FAIL, including aws_compute_sup_hides_ec2_rec.
• P3, identity canonicaliser: config FAIL (TestCanonicalSuppressionService, CanonicalisesAWSEC2Slug, LegacyEC2WithoutDetailsIsWildcard x4) and scheduler FAIL (both aws cross-slug cases).
• P4, provider check dropped: FAIL TestCanonicalSuppressionService, azure_ec2_sup_does_not_hide_compute_rec and gcp_compute_sup_does_not_hide_ec2_rec.
• P5a, wildcard on raw rec.Service==compute: FAIL LegacyEC2WithoutDetailsIsWildcard x4 and CanonicalisesAWSEC2Slug.
• P5b, wildcard on raw rec.Service==ec2: FAIL ComputeSlugEC2 and CanonicalisesAWSEC2Slug.
• P6, write side raw: FAIL LegacyEC2WithoutDetailsIsWildcard x4 and CanonicalisesAWSEC2Slug.

Commands (GOTOOLCHAIN=go1.26.10, AWS config and credentials set to /dev/null, no cloud credentials):

• go build ./...: OK.
• go vet on internal/config and internal/scheduler: OK. With -tags integration: OK.
• gofmt -l: clean.
• go test -count=1 on config and scheduler: ok. Ran -v to confirm the four new tests actually execute and PASS.
• golangci-lint run on both packages, under the heavy lock: 0 issues.
• Fail-before (head tests copied into a copy of the base export):
  • config does not build: CanonicalSuppressionService is undefined.
  • With that one test removed, TestBuildSuppressions_CanonicalisesAWSEC2Slug FAILs (3 rows, want 2).
  • TestApplySuppressions_EC2SlugSpellings FAILs on both aws cross-slug subtests.
  • Integration test TestSuppressionsMatchAcrossEC2Slugs_RealPostgres FAILs on base: ec2-rec and compute-rec stay at 5, want 3.
• Integration on head (testcontainers, nice -n 10 lockf -k -t 3600 /tmp/claude/heavy.lock):
  • full config package: ok (45s). Full scheduler package: ok (9s).
  • The new test and the fix(purchase): suppress every variant when a legacy EC2 recommendation has no details #1411 legacy test both PASS.
• git merge-tree --write-tree origin/main head:
  • clean, giving tree 27c951af, which is identical to the head tree.
  • origin/main in the fetched clone is still 5aa3df5, the base, so no merged-tree rerun was needed.
• The git diff of base..head in the clone is byte-identical to pr1419.diff.
• No migration in this PR.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

VERDICT: APPROVE 41de606

PR #1419 (Closes #1412), reviewed at head 41de606 against origin/main 5aa3df5. One commit, 6 files, +125/-7.

Findings (none blocking):

• nit internal/config/types.go:766 - only AWS ec2/compute is canonicalised; the other AWS legacy/canonical pairs that mapServiceSlug accepts (rds/relational-db, elasticache/cache, opensearch/search, redshift/data-warehouse; internal/purchase/execution.go:1268-1282) are not. If any producer ever emits an AWS canonical non-compute slug, the fix(config): purchase suppressions miss recs stored under the other EC2 service slug #1412 bug comes back for that service. I found no producer that emits one today (the frontend uses canonical slugs only for azure/gcp, frontend/src/settings.ts:143-149), so this is out of scope for the issue as written.
• nit internal/config/suppressions_build.go:35 - new rows for AWS EC2 are now written as "compute". During a rolling deploy, an old binary's applySuppressionIndex keys exact strings, so it would not hide "ec2" recs with the new rows until it is replaced. The window is one deploy and is fixed once the new code serves, so I am not blocking on it.

Attacked:

• (1) "No SQL filtering by suppressions": the only reader of purchase_suppressions is ListActiveSuppressions (internal/config/store_postgres_suppressions.go:159-166), which filters on expires_at only. Its only consumer is Scheduler.applySuppressions (internal/scheduler/scheduler.go:1408), used by ListRecommendations, ListRecommendationsStrict and GetRecommendationByID (scheduler.go:1217/1258/1294). Every delete goes by execution_id (store_postgres_suppressions.go:147, store_postgres.go:909-923). The API, dashboard and history only create or delete by execution. Plan-step resolution joins shown recs (already suppressed) to unsuppressed stored rows on RecIdentityKey on purpose (internal/planstep/resolve.go:316). The claim holds: no unnormalised suppression-matching path is left.
• (2) Blast radius of writing "compute": PurchaseSuppression.Service is read only by the scheduler's key, which is canonicalised on both sides. Grace period is keyed by provider (config/types.go:184), so it is unaffected. The UNIQUE(execution, 6-tuple, variant) constraint is safe because BuildSuppressions merges ec2 and compute into one bucket before inserting (test TestBuildSuppressions_CanonicalisesAWSEC2Slug sums 3+2). RecIdentityKey (recommendation_identity.go:61) and the rec dedupe do not use suppression rows, so they are unchanged. Down migration 000134 groups by the row's stored service, so mixed spellings are fine.
• (3) Azure and GCP "compute" pass through unchanged: the helper is gated on provider=="aws" (mutation M5 killed). Case and whitespace: validatePurchaseRecommendation lowercases provider and trims service (internal/api/validation.go:592-599). Service is not lowercased, but an "EC2" rec cannot be bought because mapServiceSlug is case-sensitive (execution.go:1268), so no suppression is written for a capacity that was actually purchased.
• (4) fix(purchase): suppress every variant when a legacy EC2 recommendation has no details #1411/fix(purchase): close two narrow double-buy windows on finalize and legacy retries #1232 wildcard: isAWSEC2 now checks the canonical b.service, which still covers both spellings. TestBuildSuppressions_LegacyEC2WithoutDetailsIsWildcard ("ec2") and TestBuildSuppressions_ComputeSlugEC2 ("compute") both pass. The real-Postgres test TestLegacyEC2RetrySuppressesBoughtVariant_RealPostgres passes.
• Mutation probes, each made in a scratch copy and reverted:
  • M1: the helper returns service unchanged. Killed by 3 config tests, the scheduler EC2SlugSpellings test, and the integration test TestSuppressionsMatchAcrossEC2Slugs_RealPostgres (got compute-rec 5 and ec2-rec 5, wanted 3 and 3).
  • M2: aggregateSuppressions reverted. Killed by 7+ scheduler tests.
  • M3: applySuppressionIndex reverted. Killed by 7+ scheduler tests.
  • M4: the suppressionBucketOf service reverted. Killed by LegacyEC2WithoutDetailsIsWildcard and CanonicalisesAWSEC2Slug.
  • M5: provider guard dropped. Killed by TestCanonicalSuppressionService and EC2SlugSpellings (the gcp ec2 case).
  • M6: wildcard check put back on rec.Service. Killed by 2 config tests.
• Runs on the exact archive of 41de606: go build ./... passes; go vet passes for config and scheduler; go test -race -short ./... shows all packages ok; go test -tags integration for config, scheduler and purchase shows all ok. The new integration test was confirmed to run, not skip (-v PASS).

Local verification at 41de6061660f0e04b465ebca89cd3fe0d3bb1d04 merged onto current main (a59740da; merge-tree 2303a088; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration passes on real Postgres (testcontainers) for the touched packages: ./internal/config ./internal/scheduler
• Nothing was applied to any cloud.